Code Review: fix(assertions): render collection contents in IsEqualTo failure messages

The fix is directionally correct and addresses a genuine UX pain point — the before/after improvement in failure message clarity is significant. Tests are well-structured and cover the important edge cases. One critical bug and one derived issue need attention before merge.

Critical: Double Enumeration in CheckAsync

File: TUnit.Assertions/Conditions/EqualsAssertion.cs

var actualPreview = FormatValue(value);          // first enumeration
if (SequenceEqualsNonGeneric(value!, _expected!)) // second enumeration of value + expected

FormatValue exhausts the enumerator for value, then SequenceEqualsNonGeneric tries to enumerate value again. For non-replayable sequences — LINQ queries, yield return generators, File.ReadLines(), IAsyncEnumerable-wrapped sources — the second pass sees an empty sequence. This causes incorrect "same contents, different reference" diagnostics even when the contents are completely different.

The same issue applies to _expected: FormatValue(_expected) in GetExpectation() runs in a separate call, potentially after CheckAsync has already consumed _expected via SequenceEqualsNonGeneric.

Better approach — buffer once, reuse:

if (_comparer is null && IsFormattableCollection(value) && IsFormattableCollection(_expected))
{
    var actualItems  = Materialize(value!);
    var expectedItems = Materialize(_expected!);
    var actualPreview = FormatItems(actualItems);

    if (SequenceEqualsItems(actualItems, expectedItems))
        return Task.FromResult(AssertionResult.Failed(
            $"received {actualPreview} (same contents, different reference — use IsEquivalentTo to compare by contents)"));

    return Task.FromResult(AssertionResult.Failed($"received {actualPreview}"));
}

// ---
private static List<object?> Materialize(object value)
{
    var list = new List<object?>();
    foreach (var item in (IEnumerable)value) list.Add(item);
    return list;
}

private static bool SequenceEqualsItems(List<object?> a, List<object?> b)
{
    if (a.Count != b.Count) return false;
    for (int i = 0; i < a.Count; i++)
        if (!Equals(a[i], b[i])) return false;
    return true;
}

private static string FormatItems(List<object?> items)
{
    var preview = string.Join(", ", items.Take(CollectionPreviewMax).Select(x => x?.ToString() ?? "null"));
    if (items.Count > CollectionPreviewMax)
        preview += $", and {items.Count - CollectionPreviewMax} more...";
    return $"[{preview}]";
}

Why is buffering the right call? It eliminates the double-enumeration hazard at the cost of a single extra allocation in the failure path — which is already allocating a string, so the relative cost is negligible. Arrays and List<T> (the common cases) aren't affected by the current bug, but the correct solution is still to enumerate once.

Minor: FormatValue iterates the full collection for the truncation count

foreach (var item in enumerable)
{
    if (total < CollectionPreviewMax) items.Add(...);
    total++;   // continues counting after cap
}

A 10,000-element array will iterate all 10,000 items just to produce "and 9,990 more...". Since this only runs on the failure path the practical impact is low, but if Materialize is adopted above the count comes for free from List.Count and the problem disappears entirely.

What's working well

• The GetExpectation() fix to call FormatValue(_expected) is the right change and gives symmetrical formatting for both sides of the message.
• The IsEquivalentTo hint when references differ but contents match is excellent discoverability — this is exactly the kind of guidance that turns a confusing failure into a self-explaining one.
• Test coverage is solid: different contents, same contents / different references, int[], List<int>, and truncation are all exercised independently.
• The 10-item cap is a sensible default; the and N more... phrasing is clear.

The double-enumeration fix is the only blocking change needed. Everything else is in good shape.

test post to verify auth

Follow-up Review: fix(assertions): render collection contents in IsEqualTo failure messages

The previous critical concern (double enumeration of non-replayable sequences) has been fully addressed. The second commit materialises both sides exactly once into List<object?> before any formatting or comparison, which is exactly the right fix. The regression test with a yield return generator confirms the fix is exercised.

Previous Issues — Resolved

Double-enumeration bug ✅ Fixed. CheckAsync now calls Materialize on both value and _expected upfront, and all subsequent operations (format, compare, cache) work against the materialized lists. GetExpectation() reuses _cachedExpectedFormat rather than re-enumerating.

Remaining Minor Observations

String items inside collections are not quoted

FormatItems calls items[i]?.ToString() directly, so a collection of strings renders as [hello, world] rather than ["hello", "world"]. This is mildly ambiguous — null and the string "null" would look the same, and whitespace-only strings are invisible. Replacing the inner format call with a small helper that quotes strings would close this gap:

private static string FormatItem(object? item) => item switch
{
    null    => "null",
    string s => $"\"{s}\"",
    _       => item.ToString() ?? "null"
};

Then in FormatItems: parts[i] = FormatItem(items[i]);

Nested collections still show type names

Items that are themselves collections (e.g. List<int[]>) are formatted via ToString() inside FormatItems, giving back System.Int32[]. FormatValue handles this case correctly via recursion but FormatItems bypasses it. Not blocking, but it means List<int[]> doesn't benefit from this fix at the item level.

FormatValue is effectively only reachable from the non-collection fallback path

The static helper FormatValue is only called from GetExpectation() when _cachedExpectedFormat is null (i.e. the collection path was not taken — either a comparer was supplied, or one side isn't a collection). Its internal Materialize call on a non-replayable IEnumerable is technically safe in that path because the collection branch in CheckAsync also would not have run. However, the method now exists in the class but is functionally unused for the common case — worth a comment or the rename to FormatFallbackValue to make the intent clear.

What's working well

• The fix is minimal and targeted — only the failure path allocates; the happy path is untouched.
• _cachedExpectedFormat cleanly bridges CheckAsync and GetExpectation() without exposing mutable state.
• SequenceEqualsLists uses object.Equals which correctly handles nulls on both sides.
• The IsEquivalentTo hint for same-contents-different-reference is an excellent UX touch.
• Test suite is thorough and includes the non-replayable-sequence regression.

The blocking issue from the prior review is resolved. The remaining items above are improvements worth considering but are not blocking merge.

Final Review: fix(assertions): render collection contents in IsEqualTo failure messages

All three rounds of feedback have been addressed in this PR. This is a solid fix that meaningfully improves test failure UX.

Previous Issues — All Resolved ✅

• Double-enumeration of non-replayable sequences ✅ — Both sides materialised once into List<object?> upfront; _cachedExpectedFormat ensures GetExpectation() reuses the already-computed string.
• String items unquoted inside collections ✅ — FormatItem now wraps strings in "" so null and the string "null" are unambiguous.
• Nested collections leaking type names ✅ — FormatItem recurses into IEnumerable via FormatItems(Materialize(enumerable)).
• FormatValue intent unclear ✅ — Comment added explaining it's the fallback path for GetExpectation() when the collection branch didn't run.

One Remaining Minor Inconsistency

SequenceEqualsLists uses object.Equals(actual[i], expected[i]) at the element level. For nested collections — e.g. int[][] where both sides have identical contents in different array instances — Equals falls back to reference equality, so SequenceEqualsLists returns false and the IsEquivalentTo hint is suppressed. The formatting is now correct (nested contents render recursively), but the discoverability hint doesn't surface for the nested case.

This is a pre-existing limitation of using EqualityComparer<T>.Default semantics and not a regression introduced here. Fixing it completely would require recursive sequence equality, which is a larger scope change. Worth a follow-up issue but not blocking.

Theoretical: FormatItem recursion on self-referential collections

FormatItem recurses into IEnumerable items via Materialize. A self-referential collection (e.g. list.Add(list)) would loop until a stack overflow. This is an edge case essentially impossible to encounter in an assertion argument, but worth being aware of if the formatter is ever exposed more broadly.

What's Working Well

• The single-materialization design is correct and clean — failure path allocates once, happy path is untouched.
• _cachedExpectedFormat bridges CheckAsync → GetExpectation() without leaking mutable state outward.
• The IsEquivalentTo hint for same-contents-different-reference is excellent discoverability UX.
• Test suite is thorough: 7 regression tests covering different types, references semantics, non-replayable sequences, truncation, string quoting, and nested collections.
• String-before-IEnumerable ordering in FormatItem correctly prevents strings being formatted as char sequences.

The PR is in good shape. The minor nested-collection hint inconsistency is the only outstanding gap, and it is pre-existing rather than regressed here. Ready to merge.